feat: Support registering evaluation suites for multiple catalogs#182
Conversation
vinayada1
force-pushed
the
feat/add-evaluation-suite-for-all-catalogs
branch
from
c7e6b27 to
e4b8878
Compare
… loaded catalogs Adds a new method to EvaluationOrchestrator that registers evaluation suites for every reference catalog loaded via AddReferenceCatalogs. This eliminates the need for plugins to re-parse catalog files to extract IDs and loop over them individually. Fixes privateerproj#181 Signed-off-by: Vinaya Damle <vinayada1@users.noreply.github.com>
vinayada1
force-pushed
the
feat/add-evaluation-suite-for-all-catalogs
branch
from
e4b8878 to
123064f
Compare
|
Overall: Clean, well-scoped PR. The new method is a reasonable convenience API and the tests are solid. A few things worth calling out: Validation gapAddEvaluationSuiteForAllCatalogs bypasses the validation that AddEvaluationSuite performs. The existing per-catalog method (line 88-102) checks:
The new method skips both checks and calls addEvaluationSuite directly. If a catalog with zero controls or an empty ID somehow ends up in referenceCatalogs, this method will silently register a broken suite while the single-catalog method would reject it. AddReferenceCatalogs does validate the ID at load time (line 61), so the empty-ID case is likely unreachable today. But the zero-controls case has no guard at load time — a catalog with metadata but no controls would be accepted by AddReferenceCatalogs and then silently registered here. Worth considering whether the new method should loop through calling the public AddEvaluationSuite(catalog.Metadata.Id, loader, steps) instead of the private addEvaluationSuite, so it gets the same validation. The trade-off is a minor performance hit from the map lookup, which is negligible. Map iteration orderThe method iterates v.referenceCatalogs (a map) which has non-deterministic order in Go. This is fine for correctness since possibleSuites ordering only matters during Mobilize, which re-orders based on config.Policy.ControlCatalogs. Just flagging that the test "Registers Suite For Each Catalog" correctly avoids asserting on order — good. Error code uniquenessThe new error code "aac10" follows a different prefix pattern than the existing ones in this file (aos10, aos20, etc.). Not a bug, but if there's a convention for these codes it'd be worth being consistent. Test coverageTests cover the three important cases (no catalogs, multiple catalogs, loader propagation). One missing case: what happens if steps is empty/nil? The method will happily register suites with no steps. Whether that's a problem depends on how Evaluate handles it downstream, but it might be worth a test or a guard. Minor nitThe test helper getTestCatalogWithID in test_data.go mutates the catalog returned by getTestCatalogWithRequirements() — this is fine since it returns a new struct each time, but it's worth verifying that getTestCatalogWithRequirements doesn't return shared pointers that could cause cross-test contamination. |
- Use public AddEvaluationSuite in AddEvaluationSuiteForAllCatalogs for proper validation - Add test for zero-controls catalog - Add tests for nil and empty steps - Refactor getTestCatalogWithID as primary test constructor Signed-off-by: Vinaya Damle <vinayada1@users.noreply.github.com>
|
Thanks for the feedback. I have fixed most of these review comments. Please let me know about the error code convention. |
2 participants
Adds a new method to EvaluationOrchestrator that registers evaluation suites for every reference catalog loaded via AddReferenceCatalogs. This eliminates the need for plugins to re-parse catalog files to extract IDs and loop over them individually.
Fixes #181